Skip to content

test: add sqllogictest coverage for DISTINCT / GROUP BY / aggregation on map columns#23406

Merged
Jefffrey merged 5 commits into
apache:mainfrom
PG1204:fix-15428-map-distinct-tests
Jul 12, 2026
Merged

test: add sqllogictest coverage for DISTINCT / GROUP BY / aggregation on map columns#23406
Jefffrey merged 5 commits into
apache:mainfrom
PG1204:fix-15428-map-distinct-tests

Conversation

@PG1204

@PG1204 PG1204 commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

SELECT DISTINCT on map columns used to fail with ArrowError(NotYetImplemented) because arrow-rs's RowConverter did not support Map types. That support has since landed upstream (apache/arrow-rs#7879) and is available on DataFusion main via arrow 59.1, so these queries now work. As suggested in #15428 (comment) (maintainer comment), this PR adds sqllogictest coverage to lock in the behavior and close the issue.

What changes are included in this PR?

Adds a new test section to datafusion/sqllogictest/test_files/map.slt covering:

  • SELECT DISTINCT on a map column, including the exact reproducer from the issue (DISTINCT ... LIMIT), collapsing of duplicate maps and duplicate NULLs, and DISTINCT over map + scalar columns together
  • GROUP BY on a map column, GROUP BY map + scalar keys, and HAVING with a map grouping key
  • Aggregations with map inputs: COUNT(map), COUNT(DISTINCT map), SUM grouped by map, and array_agg of map values
  • Edge cases: empty maps under DISTINCT (equal to each other, distinct from NULL), NULL maps, and unsorted maps with the same entries in different order being treated as distinct values
  • UNION (distinct) on map columns
  • DISTINCT / GROUP BY over the existing 209-row parquet_map.parquet file for coverage on real Parquet-backed map data

Note: a bare MAP {'key': NULL} literal infers a Null value type and fails type coercion across a VALUES list against Map(Utf8, Int64) rows, so the test uses CAST(NULL AS BIGINT) for the null map value. That coercion gap may be worth a separate follow-up issue.

Are these changes tested?

Yes, this PR is test-only. All tests pass locally via cargo test -p datafusion-sqllogictest --test sqllogictests -- map.slt.

Are there any user-facing changes?

No. Test-only change; no API or behavior changes.

@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Jul 9, 2026
@Jefffrey

Copy link
Copy Markdown
Contributor

thanks @PG1204

Merged via the queue into apache:main with commit 761f7f6 Jul 12, 2026
36 checks passed
adriangb pushed a commit to pydantic/datafusion that referenced this pull request Jul 15, 2026
…S lists (apache#23521)

## Which issue does this PR close?

- Closes apache#23474.

## Rationale for this change

A bare `MAP {'k1': NULL}` literal infers a `Null` value type, producing
`Map("entries": Struct("key": Utf8, "value": Null))`. Placing it in a
VALUES list alongside concretely-typed map rows failed with:

Error during planning: Inconsistent data type across values list at row
1 column 0.

This is because `type_union_resolution_coercion` (the pairwise worker
behind `type_union_resolution`, which the VALUES builder uses to widen
row types) had recursion arms for `Dictionary`, `RunEndEncoded`,
`Struct`, and `List` (via `list_coercion`), but none for `Map`. Scalar
NULL inference across a VALUES list (`VALUES (1), (NULL)`) already
worked; the same inference just never reached into a map's value type.
This gap was noted in apache#23406, where the tests had to use a `CAST(NULL AS
BIGINT)` workaround.

## What changes are included in this PR?

- A one-line fix in
`datafusion/expr-common/src/type_coercion/binary.rs`: wire the existing
`map_coercion` helper (already used by `comparison_coercion` and
`type_union_coercion`) into `type_union_resolution_coercion`'s fallback
chain. The recursion into the map's key/value types then applies the
exact same rules as scalar VALUES coercion (`null_coercion` for NULL
inference, `binary_numeric_coercion` for numeric widening),
symmetrically in either row order.
- A unit test (`test_type_union_resolution_map`) covering Null + Int64
value types in both orders, Int64 + Null + Float64 widening, and Map vs.
Struct still refusing to unify.
- A new "map NULL value coercion in VALUES" section in
`datafusion/sqllogictest/test_files/map.slt` covering: the issue
reproducer, the reversed row order, all-NULL values (resulting type
`Map("entries": Struct("key": Utf8, "value": Null))`, verified via
`arrow_typeof`), multi-key rows with one NULL value, NULL scattered
across three rows, numeric widening to Float64, two-level nested maps,
NULL round-tripping as NULL through `SELECT`, and `INSERT INTO ...
VALUES`.
- Incompatible concrete value types still error: `(MAP {'k': 1}), (MAP
{'k': 'hello'})` follows the scalar VALUES rule (coerce to the numeric
type, then fail with `Cast error: Cannot cast string 'hello' to value of
Int64 type`, which is the same behavior as `VALUES (1), ('hello')`).
- A note above the apache#23406 tests documenting that the `CAST(NULL AS
BIGINT)` workaround is no longer required (those tests are left
unchanged).

## Are these changes tested?

Yes. New unit test in `datafusion-expr-common` and new sqllogictests in
`map.slt` as described above. The full sqllogictest suite (492 files)
passes locally with zero regressions, as do `cargo test -p
datafusion-expr -p datafusion-expr-common`, `cargo fmt`, and `cargo
clippy --workspace --all-targets -- -D warnings`.

## Are there any user-facing changes?

`VALUES` lists (and other contexts that use `type_union_resolution`,
such as `CASE`/`COALESCE`/array literals) now unify Map types whose
key/value types are coercible, including bare NULL map values. No API
changes; previously failing queries now succeed, and incompatible-type
errors are preserved.

---------

Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

select distinct on map types yields ArrowError NotYetImplemented

2 participants